Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow editing "extended_settings" user property #1160

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DEVTomatoCake
Copy link
Member

This PR just adds the user property extended_settings to the user modify schema, so applications can actually use it to store extended settings.

@MaddyUnderStars
Copy link
Member

I'm not sure about this one @Puyodead1

@Puyodead1
Copy link
Contributor

I'm not sure about this one @Puyodead1

why?

@MathMan05
Copy link

Extending settings is just a string? What if two clients both want to store extended settings, does that mean they just fight over it?

@MathMan05
Copy link

Maybe a better idea would be it being a {[key:String]:String} to let clients use the extended settings as an object so conflicts are generally more graceful

@DEVTomatoCake
Copy link
Member Author

Extending settings is just a string? What if two clients both want to store extended settings, does that mean they just fight over it?

It's "{}" by default, so you're expected to read and store it as JSON - if a client doesn't do that or overwrites stuff there's nothing the server can do really

@MathMan05
Copy link

Ok

@MaddyUnderStars
Copy link
Member

I'm not sure about this one @Puyodead1

why?

Not sure why I didn't get an email for this reply, but I'm concerned that a malicious application could just, upload some large data and try to harm the server/database. There's no restrictions at all on the kind of data stored here, which is my problem. Also, I'm not sure what applications would even use this for. Client apps can very easily just store settings locally and allow some sort of export functionality.

@Puyodead1
Copy link
Contributor

I'm not sure about this one @Puyodead1

why?

Not sure why I didn't get an email for this reply, but I'm concerned that a malicious application could just, upload some large data and try to harm the server/database. There's no restrictions at all on the kind of data stored here, which is my problem. Also, I'm not sure what applications would even use this for. Client apps can very easily just store settings locally and allow some sort of export functionality.

ah, makes sense.

@MathMan05
Copy link

Yeah, clients can just store the information locally, but like clients like mine that have more themes, or stuff like that, it'd make sense to store that non-locally

@MathMan05
Copy link

can this please get merged? I'd like to have this for storing some settings, it doesn't have to get merged now, I'd just like to know when this should happen ish.

@Puyodead1
Copy link
Contributor

can this please get merged? I'd like to have this for storing some settings, it doesn't have to get merged now, I'd just like to know when this should happen ish.

just store settings in localstorage?

@DEVTomatoCake
Copy link
Member Author

can this please get merged? I'd like to have this for storing some settings, it doesn't have to get merged now, I'd just like to know when this should happen ish.

just store settings in localstorage?

Why do you want to force users to reconfigure all custom settings every time they're on new device or clear the browser cache?

This rather encourages off-Spacebar storage which also gets the job done, but shouldn't be used... If there's already a place to store user settings, why not also store client specific settings? I'll happily implement some limits for this, but in general I'd prefer this over localStorage/external storage

@MathMan05
Copy link

Or a thought I've had several times to just store this kind of stuff in the dms to yourself, which I'm not going to do as that'd be really jank and likely look bad on other clients

@MaddyUnderStars
Copy link
Member

Another problem I have with this is that allowing clients to edit extended settings directly makes it more difficult for clients to use it, because now they have to worry about not breaking other client's stored settings. It would be better if there was a system to register an app's settings to that specific app so that it doesn't conflict with other apps. This still doesn't fix a malicious client potentially wiping settings or adding things to other apps though. E.g. if we had a naive solution where when updating extended settings, you also provide your app name and the server just does extended_settings[app_name] = Object.assign(extended_settings[app_name], new_data), then it'd be trivial for a malicious app to look at another apps source code and just use the same app id there.

Is this even a problem we need to think about? I suppose if we were in discord's position, and client mods made use of extended settings, then it'd be possible for like, a malicious app to add like background: url(ip_grabber_image) to your custom css or whatever. Obviously that's a hypothetical, but yeah.

Why do you want to force users to reconfigure all custom settings every time they're on new device or clear the browser cache?

How often does that happen? But also, isn't this what the normal settings are for? I think those cover a good majority of use cases, and clients should use them. I have no problem with that, since the server controls exactly what type of data is stored in there, and we can do validation very easily with that. Extended settings as implemented here is just a plain json object that we have no control over.

@DEVTomatoCake
Copy link
Member Author

then it'd be possible for like, a malicious app to add like background: url(ip_grabber_image) to your custom css or whatever.

Sure, but it could also just view the (current) IP itself. That's an issue all clients have, and kinda the users fault if they use such a client imo

How often does that happen?

I'm developing Jank Client on two, sometimes even three different devices - so, quite often for me.

But also, isn't this what the normal settings are for?

Technically yes, but I don't think these are going to support e.g. custom CSS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants